Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added code to calculate crt for non-coprime moduli in Integer #39716

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

Noel-Roemmele
Copy link
Contributor

Related to issue #32487. Added code to compute crt in Integer when the two moduli are not coprime. Also changed error produced when a solution does not exist. Error message is based of the error message found in the crt method in src\sage\arith\misc.py.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Mar 16, 2025

Documentation preview for this PR (built with commit 8cba058; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@DaveWitteMorris
Copy link
Member

In line 6965, you need to change mn to lcm(m,n) (but in latex mode). I think you have two options:

  1. Change mn to \\lcm(m,n)
  2. Add r just before """ at the start of the previous line (to make it a raw string), and change mn to \lcm(m,n)

In line 6978, you need double-backticks, not single-backticks, around crt.

In line 6995, please change <> to !=.

I think line 6996 will be more readable if you use an f-string to create the error message: f"no solution to crt problem since gcd({_m},{_n}) does not divide {self} - {_y}"

This is very minor, but you reversed the order of _m and _n at the end of line 6997 and I think it would be better not to do that, since there is no need to change the order: _m.lcm(_n) is slightly better than _n.lcm(_m).

@Noel-Roemmele
Copy link
Contributor Author

@DaveWitteMorris I've made the changes suggested. I also added the modulo _m.lcm(_n) to the case where m and n are co-prime as we changed the documentation to specify that. Let me know if I should change it back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants